-
Notifications
You must be signed in to change notification settings - Fork 917
add support for request_get_status_any/all/some #13279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
d233c73
to
723b84e
Compare
Let me know if you want help with the python binding infrastructure. |
@hppritcha yes, I would appreciate some help on that front (and the same applies for the fortran bindings, even if its just a conversation that I understand what needs to be done). |
efd9cf5
to
f21dff7
Compare
@edgargabriel added new data type to the bindings code to add this new use of |
I'll work on the fortran but that will have to wait till later in the week. |
note that mpi4py is probably not testing this since it keys off our MPI_Get_version return values for major/minor. |
@hppritcha thank you very much, I will test it later this week. I have not done the mpi4py test that you mentioned, will look into this as well. |
a2aa8f9
to
81e3492
Compare
faking MPI 4.1. for mpi4py doesn't work unfortunately, we fail the compilation of mpi4py due to other missing function (MPI_Buffer_flush etc.). I will write some tests for the new functions. |
@edgargabriel i pushed the remainder of the fortran related files to the PR - hopefully. |
@hppritcha thank you! I hope to finish up adding tests to the ibm testsuite for the new function this weekend, and hopefully next week we could merge the PR if all tests pass. |
I added some tests for request_get_status_any/all/some to the ompi-tests private testsuite, and they pass. This is however only testing the C-interfaces of the function. The tests are also probably not entirely bullet-proof in terms of that they might miss some corner cases, but the base function is shown to be working. |
360c797
to
47fbc39
Compare
2963f48
to
f326157
Compare
did we add the tests for this to ompi-tests? i'm triaging compile failures for the tests in my mtt runs:
|
@hppritcha I added these tests to the ibm testsuite https://github.com/open-mpi/ompi-tests/pull/189 |
It was probably too early to merge it, should have waited until we merge the actual code. Sorry, I didn't think it through :-( |
related to #13279 |
We only support named types with this first pass. related to open-mpi#12076 Fortran interfaces will be added once Fortran infrastructure additions in PR open-mpi#13279 are merged in to main. Signed-off-by: Howard Pritchard <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Can we squash these down (don't have to squash to 1 commit -- just get rid of the "fixup" commits)? Then let's merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are some major issues with this PR. Please check my comments.
** MPI functions invoking this functionality does not have the const | ||
** argument | ||
*/ | ||
if(!ompi_request_check_same_instance((MPI_Request *)requests, count) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing ompi_request_check_same_instance
to use const make sense as part of this PR because it is the first time it is useful to have const in the API instead of casting everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bosilca just to clarify: If I would change the ompi_request_check_same_instance
function to take const MPI_Request *
as an argument, I would have to add typecasts to MPI_Waitall/Waitany/Waitsome/Testall/Testany/Testsome
since they are not const MPI_Request*
. Is that what you are suggesting?
My line of thinking was that even if we decide to make that change (which I don't think I am technically in favor), I would prefer to do that in a separate PR, since I didn't want to touch the Waitall/Testall and friends functions in this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I understand why you would have to make changes anywhere else? Passing a non-const
to a const
function argument does not require casting (unlike passing a const
to a non-const
).
My concern is that delayed things have a tendency to remain delayed and then forgotten because there is always something more important. This is a general statement, I'm not pointing fingers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that is correct. I made change that you suggested, and now I get a lot of warnings from the files that I mentioned:
testall_generated.c: In function 'PMPI_Testall':
testall_generated.c:64:50: warning: passing argument 1 of 'ompi_request_check_same_instance' from incompatible pointer type [-Wincompatible-pointer-types]
64 | if(!ompi_request_check_same_instance(requests, count) ) {
| ^~~~~~~~
| |
| struct ompi_request_t **
In file included from ../../../ompi/communicator/comm_request.h:18,
from ../../../ompi/communicator/communicator.h:45,
from testall_generated.c:33:
../../../ompi/request/request.h:575:62: note: expected 'const ompi_request_t **' but argument is of type 'struct ompi_request_t **'
575 | bool ompi_request_check_same_instance(const ompi_request_t** requests,
| ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bosilca, I incorporated all of your change requests except for this const thing, can you please re-check and let me know how you would like to proceed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm puzzled by the error message as I don't see why we cannot pass a non-const argument to a const function. Anyway, as is this PR is good to go.
- add the man pages for the newly implemented MPI_Request_get_status_all/any/some functions. - c-bindings: add support for const request args Three new functions were added to the MPI API as part of the 4.1 standard. These used an array of const MPI_Request s which the bindings code didn't support. This commit also fixes back the mpi.h prototypes to include the constants and required changes to the new template files. - request_get_status variants: add f08 interfaces also switch MPI_Request_get_status to use new method for generating f08 bindings. - Update fortran bindings interfaces generation code. - f90/f77 interfaces will be added as another commit. - use-mpi-ignore: start using binding infrastructure - request_get_status_mult: add f77 functions Signed-off-by: Edgar Gabriel <[email protected]> Signed-off-by: Howard Pritchard <[email protected]>
88d7f08
to
b057513
Compare
OMPI_COPY_STATUS(status, requests[i]->req_status, false); | ||
} | ||
return MPI_SUCCESS; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else
itself is unnecessary, it can be assumed from the rest of the checks. That's how it is done in the some
function.
** MPI functions invoking this functionality does not have the const | ||
** argument | ||
*/ | ||
if(!ompi_request_check_same_instance((MPI_Request *)requests, count) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm puzzled by the error message as I don't see why we cannot pass a non-const argument to a const function. Anyway, as is this PR is good to go.
We only support named types with this first pass. related to open-mpi#12076 Fortran interfaces will be added once Fortran infrastructure additions in PR open-mpi#13279 are merged in to main. Signed-off-by: Howard Pritchard <[email protected]>
Add support for the new
MPI_Request_get_status_any/all/some
functions introduced with MPI 4.1This PR contains the C implementation for the functions as well as the man-pages.
There are however also some things missing:
const MPI_Request[]
, (I have currently removed theconst
from the function definitions in mpi.h.in in order to make the code compile).